-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
docs(js): streamline React Router v6 and v7 documentation #13138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
docs/platforms/javascript/guides/react/features/react-router/v6.mdx
Outdated
Show resolved
Hide resolved
docs/platforms/javascript/guides/react/features/react-router/v6.mdx
Outdated
Show resolved
Hide resolved
docs/platforms/javascript/guides/react/features/react-router/v6.mdx
Outdated
Show resolved
Hide resolved
docs/platforms/javascript/guides/react/features/react-router/v6.mdx
Outdated
Show resolved
Hide resolved
coolguyzone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🤙
docs/platforms/javascript/guides/react/features/react-router/v6.mdx
Outdated
Show resolved
Hide resolved
| - Using `wrapCreateBrowserRouterV6` or `wrapCreateMemoryRouterV6` | ||
|
|
||
| </Alert> | ||
| _React Router v6 support is included in the `@sentry/react` package since version `7`._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| _React Router v6 support is included in the `@sentry/react` package since version `7`._ |
IMHO we can drop this, we are on v9 now 😅
| description: "Learn about Sentry's React Router v6 integration." | ||
| sidebar_order: 20 | ||
| --- | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to lead with something like this:
| You can instrument your React Router v6 setup with Sentry. Depending on how you use react-router, slightly different setups are needed: | |
| <TableOfContents ignoreIds={["custom-error-boundary", "next-steps"]} /> | |
| Additionally, make sure to [Setup a Custom Error Boundary](#custom-error-boundary) to ensure rendering errors are captured to Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and something similar for react-router v7 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mydea Good point! I updated the start of the page with an adapted version of your suggestion.
Sadly, the TableOfContents component does not work with our structure as it seems to be created to work with the SDK options pages, and our structure here is quite different -> I created an issue #13159 so we can consider upgrading the functionality of this component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be fixed here #13171 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the toc is now included - looks good!
| need error boundaries to handle errors in event handlers. | ||
| </Alert> | ||
|
|
||
| To send errors to Sentry while using a custom error boundary, use the `Sentry.captureException` method: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to any change in this PR but generally: We should clarify here, because it is not clear to me: What If I am not using a custom error boundary, what should I do then? Should I add one to capture errors, or not? cc @lforst & @AbhiPrasad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for react router you need to add one, otherwise component-related errors get swallowed by the router itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, so let's reword this (can be a different PR as well) to make it clear you def should add this, not just if you're using a custom boundary!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a sentence to the first paragraph in this section (v6 and v7 pages) -- hope this is what you meant (if not, let's chat later today @mydea :) )
…act/router-clean-up
Bundle ReportChanges will decrease total bundle size by 15 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: sentry-docs-client-array-pushAssets Changed:
view changes for bundle: sentry-docs-server-cjsAssets Changed:
|
…act/router-clean-up
| Now, Sentry should generate `pageload`/`navigation` transactions with parameterized transaction names (for example, `/teams/:teamid/user/:userid`), where applicable. This is only needed at the top level of your app, unlike React Router v4/v5, which required wrapping every `<Route />` you wanted parametrized. | ||
|
|
||
| ### Custom Error Boundaries | ||
| ## Usage With Custom Error Boundaries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ## Usage With Custom Error Boundaries | |
| ## Set Up A Custom Error Boundary |
IMHO I would rephrase this like this, as everybody should follow this step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this on v6 and v7 pages
| <Alert> | ||
| Note, that this only applies to render method and lifecycle errors since React | ||
| doesn't need error boundaries to handle errors in event handlers. | ||
| This only applies to render method and lifecycle errors since React doesn't | ||
| need error boundaries to handle errors in event handlers. | ||
| </Alert> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this section helpful? What does it tell me? 😅 I should set this up anyhow to get all errors, I'd say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this whole alert here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it vor v6 and v7 👍
| Now, Sentry should generate `pageload`/`navigation` transactions with parameterized transaction names (for example, `/teams/:teamid/user/:userid`), where applicable. This is only needed at the top level of your app, unlike React Router v4/v5, which required wrapping every `<Route />` you wanted parametrized. | ||
|
|
||
| ### Custom Error Boundaries | ||
| ## Usage With Custom Error Boundaries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above?
| In production, these errors won't surface unless captured manually. If you **don't** have a custom error boundary in place, `react-router` will create a default one that "swallows" all errors.\ | ||
| Hence, to capture these errors with Sentry in production, we strongly recommend to implement a custom error boundary. | ||
|
|
||
| <Alert> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, do we need this alert?
DESCRIBE YOUR PR
Updated the React Router v6 and v7 documentation:
IS YOUR CHANGE URGENT?
Help us prioritize incoming PRs by letting us know when the change needs to go live.
SLA
Thanks in advance for your help!
PRE-MERGE CHECKLIST
Make sure you've checked the following before merging your changes:
EXTRA RESOURCES